Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
Jacobfaib
force-pushed
the
jacobf/2026-04-07/performance-unnecessary-value-param
branch
from
849453d to
b53da5d
Compare
Jacobfaib
requested review from
elstehle,
ericniebler,
srinivasyadav18 and
wmaxey
fbusato
left a comment
fbusato
left a comment
There was a problem hiding this comment.
not sure if it is worth for CUB. The PR applies NOLINT everywhere and use move which just adds verbosity
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Jacobfaib
force-pushed
the
jacobf/2026-04-07/performance-unnecessary-value-param
branch
from
c6bb09e to
9ccde38
Compare
This comment has been minimized.
This comment has been minimized.
Jacobfaib
force-pushed
the
jacobf/2026-04-07/performance-unnecessary-value-param
branch
from
9ccde38 to
580cbd9
Compare
This comment has been minimized.
This comment has been minimized.
jrhemstad
left a comment
jrhemstad
left a comment
There was a problem hiding this comment.
I'd be very cautious about applying this too widely on device code functions. I've routinely seen passing arguments by const& instead of by value lead to increased local memory usage because ODR using the value by taking a reference can lead to the compiler spilling registers to local memory. Given the number of places we've had to manually disable this check, my intuition is it'd be better to just omit this check altogether.
|
I believe this PR is too big to properly review. We should split this up into smaller parts per subproject. Also I second @jrhemstad that using references is sometimes a loss, especially for types that are cheap to copy |
|
@jrhemstad @miscco see the PR description, I took great care to not use references anywhere that would reasonably be used on device more often than not (e.g. CUB, or the thrust kernels). These are all silenced. But yeah, given how much of these diffs are just |
Jacobfaib
force-pushed
the
jacobf/2026-04-07/performance-unnecessary-value-param
branch
3 times, most recently
from
efe38e6 to
9b5f22f
Compare
This comment has been minimized.
This comment has been minimized.
Jacobfaib
force-pushed
the
jacobf/2026-04-07/performance-unnecessary-value-param
branch
from
c97e1c9 to
7509fe4
Compare
😬 CI Workflow Results🟥 Finished in 3h 19m: Pass: 99%/428 | Total: 14d 16h | Max: 3h 08m | Hits: 73%/555163See results here. |
I think this is mostly concisely targeting Thrust. I like the idea from a correctness point of view, but understand the difficulty with actually narrowing the check to just host API. I won't immediately say no to the bulk of this. |
5 participants
Description
To silence this check you can either:
My policy for fixing this was as follows:
The upshot is that this was almost never fixed via const-ref. The vast majority of violations in CUB are silenced, and a lot of them are fixed via moves in thrust.
Checklist